Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds view_closed support #276

Merged
merged 14 commits into from
Oct 7, 2019

Conversation

shaydewael
Copy link
Contributor

Summary

Addresses #263 and #266

Requirements (place an x in each [ ])

@shaydewael shaydewael requested review from aoberoi and seratch October 3, 2019 21:36
@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

Merging #276 into master will increase coverage by 1.06%.
The diff coverage is 45.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
+ Coverage   72.65%   73.71%   +1.06%     
==========================================
  Files           7        7              
  Lines         501      506       +5     
  Branches      142      145       +3     
==========================================
+ Hits          364      373       +9     
+ Misses        105      101       -4     
  Partials       32       32
Impacted Files Coverage Δ
src/helpers.ts 92.3% <100%> (ø) ⬆️
src/ExpressReceiver.ts 52.77% <100%> (ø) ⬆️
src/App.ts 79.19% <40%> (-2.3%) ⬇️
src/middleware/builtin.ts 68% <43.33%> (+6.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e11041...c4114be. Read the comment docs.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/App.ts Show resolved Hide resolved
src/middleware/builtin.ts Outdated Show resolved Hide resolved
src/middleware/builtin.ts Show resolved Hide resolved
src/types/view/index.ts Outdated Show resolved Hide resolved
@@ -437,20 +437,19 @@ describe('App', () => {
respond: noop,
ack: noop,
},
// TODO: https://github.com/slackapi/bolt/issues/263
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we also can verify if the condition works by having the following lines in the test suite.

// add a new routing definition
app.view({callback_id: 'view_callback_id', type: 'view_closed'}, ({ }) => { viewFn(); })

// modify assert.equal(viewFn.callCount, 1); as below:
assert.equal(viewFn.callCount, 2);

@shaydewael shaydewael merged commit 5ed8c89 into slackapi:master Oct 7, 2019
@shaydewael shaydewael deleted the add-view-closed-support branch October 7, 2019 23:29
Comment on lines +473 to +474
app.view('view_callback_id', ({ }) => { viewFn(); });
app.view({ callback_id: 'view_callback_id', type: 'view_closed' }, ({ }) => { viewFn(); });
Copy link

@rtrembecky rtrembecky Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi, I was looking for a way to listen to view closed actions and I stumbled upon this PR.
can you please explain why this doesn't trigger multiple listeners? why is the view_closed action not handled by app.view('view_callback_id', ...) as well?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to write a listener to handle both the submission and the close action in the same function and I expected to write app.view('view_callback_id', ...) so it catches both 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rtrembecky

can you please explain why this doesn't trigger multiple listeners?

No, both listeners are triggered here. The following assertion assert.equal(viewFn.callCount, 2); verifies if viewFn is called twice.

Thus, as long as you set only callback_id to an app.view listener, the listener should work as you expect.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the quick reply. however, that's not what I experience. I have the following listeners:

this.boltApp.view(FEEDBACK_NOTE_CALLBACK_ID, ({body, ack}) => this.handleFeedbackNoteCallback(body, ack, 'first'))
this.boltApp.view({callback_id: FEEDBACK_NOTE_CALLBACK_ID, type: 'view_closed'}, ({body, ack}) => this.handleFeedbackNoteCallback(body, ack, 'second'))

I'm logging the "first" and "second" strings. When closing a modal, I just tested only the second function is called.
I'm on "@slack/bolt": "^3.8.1",. Maybe this is a regression or something 🤷‍♂️ But I don't have time to create an issue or investigate further, as I have the solution that works. Thanks for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants